Skip to content

Support ethdebug source locations under EOF #15994

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 8 commits into from
Jun 11, 2025
Merged

Support ethdebug source locations under EOF #15994

merged 8 commits into from
Jun 11, 2025

Conversation

clonker
Copy link
Member

@clonker clonker commented Apr 10, 2025 •
edited
Loading

  • Adds an EthdebugSchema header with the relevant part of the schema mapped to structs with corresponding to_json methods and validations
  • Skips the context for instructions with source locations that are not valid (ie, (-1, -1))
  • Adds source location info in evmasm assembly for assembleEOF
  • Refactors legacy assemble to use RAII-style instruction location gathering

Fixes the unoptimized part of #15978.
Fixes #15998.

@clonker clonker force-pushed the eof_source_locations_unoptimized branch 2 times, most recently from 20d6a64 to fb559fb Compare April 11, 2025 09:12
@clonker clonker marked this pull request as ready for review April 11, 2025 09:38
@clonker clonker force-pushed the eof_source_locations_unoptimized branch 9 times, most recently from e6a1055 to 2a91773 Compare April 12, 2025 06:49
@clonker clonker requested review from cameel and aarlt April 12, 2025 09:12
@clonker clonker force-pushed the eof_source_locations_unoptimized branch 5 times, most recently from d82b70c to 0fd1232 Compare April 15, 2025 07:16
@clonker clonker mentioned this pull request Apr 17, 2025
Merged
@clonker clonker force-pushed the eof_source_locations_unoptimized branch from 0fd1232 to cf6aa71 Compare April 17, 2025 08:18
@clonker clonker requested a review from matheusaaguiar April 17, 2025 09:11
matheusaaguiar
matheusaaguiar previously approved these changes Apr 22, 2025
View reviewed changes
Copy link
Collaborator

@matheusaaguiar matheusaaguiar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

cameel
cameel reviewed Apr 22, 2025
View reviewed changes
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to take a closer look at this, especially the Assembly.cpp part, but for now just a few small annoyances I found while doing a quick initial pass.

test/cmdlineTests/standard_output_debuginfo_ethdebug_compatible_eof/output.json Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The output here is quite long and seems to have little to do with ethdebug itself (the ethdebug JSON gets stripped). Do we need it all? What's the point?

Copy link
Member Author

@clonker clonker Apr 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I just added it because there is the same test for non-eof. And looking at it now, I agree. There doesn't seem to be much value to keep it (or: them) around, especially with #16009 around the corner. I have removed this one.

@clonker clonker force-pushed the eof_source_locations_unoptimized branch 3 times, most recently from 3c02df0 to f9a0985 Compare April 23, 2025 06:51
@clonker clonker requested a review from cameel April 28, 2025 15:44
aarlt
aarlt previously approved these changes May 9, 2025
View reviewed changes
nikola-matic
nikola-matic reviewed May 14, 2025
View reviewed changes
@clonker clonker force-pushed the eof_source_locations_unoptimized branch from 667d2e3 to bcf1c34 Compare May 14, 2025 15:16
cameel
cameel reviewed May 14, 2025
View reviewed changes
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only looked at the Assembly changes so far, but they look good. We could merge that if you extract it into a separate PR.

Changelog.md
@@ -4,7 +4,7 @@ Language Features:


Compiler Features:

* ethdebug: Experimental support for instructions and source locations under EOF.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* ethdebug: Experimental support for instructions and source locations under EOF.
* ethdebug: Experimental support for instructions and source locations under EOF.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uhh. Happy to fix this - but why? It's not like there's a blank line between bullet point list and headline anywhere else.

Copy link
Member

@cameel cameel May 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, my bad, should have been the other way around - an extra empty line below :)

but why?

Two empty lines between sections.

Maybe we should just change the convention to a single line? No one seems to be able to keep it straight or see it in the diff and it gets changed by PRs back and forth. I usually ignore it but I got annoyed enough seeing it again to suggest a correction :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha yeah, that explains it. Might've also been my pre-coffee confusion that I didn't realize you meant the blank line below. I am absolutely fine with changing it to one line in any case! :)

@clonker clonker force-pushed the eof_source_locations_unoptimized branch 3 times, most recently from af84571 to aad6fca Compare May 15, 2025 09:08
@clonker clonker mentioned this pull request May 15, 2025
Merged
@clonker clonker force-pushed the eof_source_locations_unoptimized branch 2 times, most recently from 2f4f25b to 0ca8800 Compare May 21, 2025 08:19
@clonker clonker requested review from nikola-matic and aarlt May 21, 2025 08:20
@clonker
Copy link
Member Author

clonker commented May 21, 2025

The assembler stuff has been merged with #16052, which leaves this PR in a state in which it solely adds ethdebug features

@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 6, 2025

This pull request is stale because it has been open for 14 days with no activity.
It will be closed in 7 days unless the stale label is removed.

@github-actions github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Jun 6, 2025
@clonker clonker removed the stale The issue/PR was marked as stale because it has been open for too long. label Jun 10, 2025
@clonker clonker force-pushed the eof_source_locations_unoptimized branch from 0ca8800 to dd25b24 Compare June 11, 2025 09:35
matheusaaguiar
matheusaaguiar approved these changes Jun 11, 2025
View reviewed changes
Copy link
Collaborator

@matheusaaguiar matheusaaguiar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few nitpicks, nothing that would block.

@clonker clonker merged commit a9df00c into develop Jun 11, 2025
74 checks passed
@clonker clonker deleted the eof_source_locations_unoptimized branch June 11, 2025 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cameel cameel cameel left review comments

@gnidan gnidan gnidan left review comments

@aarlt aarlt aarlt approved these changes

@matheusaaguiar matheusaaguiar matheusaaguiar approved these changes

@nikola-matic nikola-matic Awaiting requested review from nikola-matic

Assignees
No one assigned
Labels
ethdebug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

EthDebug source range offsets must be non-negative
6 participants
@clonker @aarlt @cameel @gnidan @nikola-matic @matheusaaguiar